460: fix: Enum and Fixed would put `default` in `custom_attributes` by martin-augment · Pull Request #47 · martin-augment/avro-rs

martin-augment · 2026-02-11T20:32:31Z

460: To review by AI

gemini-code-assist · 2026-02-11T20:32:46Z

Summary of Changes

Hello @martin-augment, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a bug where the default field for Enum and Fixed Avro schemas was incorrectly being treated as a custom attribute during serialization and parsing. The changes ensure that these standard Avro properties are handled explicitly, preventing their misclassification and ensuring correct schema representation. This improves the fidelity of schema round-tripping and adherence to the Avro specification.

Highlights

Serialization Fix for Fixed Schemas: The serialization logic for FixedSchema was updated to correctly include the default field, ensuring it is serialized as a standard Avro property rather than being absorbed into custom attributes.
Serialization Fix for Enum Schemas: The serialization logic for Enum schemas was modified to explicitly serialize the default and doc fields, preventing them from being incorrectly treated as custom attributes.
Parsing Correction for Custom Attributes: The schema parser was adjusted to correctly exclude the default field from custom_attributes when parsing both Enum and Fixed schemas, aligning with Avro specification.
New Test Cases: New unit tests were added to validate that default and doc fields for Fixed and Enum schemas are properly handled and not mistakenly included in custom_attributes after serialization and deserialization.

Changelog

avro/src/schema/mod.rs
- Updated FixedSchema serialization to explicitly include the default field.
- Refactored Option unwrapping in FixedSchema serialization from ref n to n.as_ref() for consistency.
- Modified Schema::Record pattern matching to explicitly destructure lookup.
- Updated Schema::Enum pattern matching to explicitly destructure default and doc fields.
- Enhanced Schema::Enum serialization to explicitly include default and doc fields.
- Added new test cases (avro_rs_460_fixed_default_in_custom_attributes and avro_rs_460_enum_default_in_custom_attributes) to verify correct handling of default and doc fields.
avro/src/schema/parser.rs
- Modified Parser::parse_enum to exclude 'default' from custom attributes when calling get_custom_attributes.
- Refactored default length check in Parser::parse_fixed to use if let Some(default) = &default for improved clarity and efficiency.
- Modified Parser::parse_fixed to exclude 'default' from custom attributes when calling get_custom_attributes.

Activity

The pull request was marked for review by AI.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

coderabbitai · 2026-02-11T20:33:00Z

Walkthrough

This pull request modifies Avro schema serialization and parsing logic. FixedSchema serialization now conditionally includes a "default" field when present and uses borrowed references for optional fields. Record and Enum variant serialization changes include "default" and "doc" fields in emitted JSON when provided. The parser extends custom attribute extraction to exclude "default" for Enum and Fixed schemas, treating it separately from custom attributes. Parser changes also adopt borrowed patterns instead of clone-based approaches. Tests are added to verify round-tripping of fixed and enum schemas with defaults and documentation preserved. No public API signatures are altered.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch pr-460-2026-02-11-20-32-27

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

claude · 2026-02-11T20:34:18Z

PR Review: Fix Enum and Fixed Default in Custom Attributes

Summary

This PR fixes a bug where default values for Enum and Fixed schema types were incorrectly being included in custom_attributes instead of being treated as proper schema fields. The fix ensures proper serialization/deserialization round-trips.

Code Quality: ✅ Good

Strengths:

Clean, focused changes that address the specific issue
Follows existing code patterns in the codebase
Good use of idiomatic Rust patterns (e.g., as_ref() instead of ref)

Minor observations:

Parser changes (avro/src/schema/parser.rs:693, 812): The fix correctly adds "default" to the excluded list in get_custom_attributes() for both enum and fixed types. This ensures default is treated as a first-class schema field rather than a custom attribute.
Serialization improvements (avro/src/schema/mod.rs):
- Fixed schema: Added serialization of default field (lines 310-312)
- Enum schema: Added serialization of both default and doc fields (lines 829-834)
- The destructuring change from .. to explicitly named _lookup and listing default, doc improves code clarity and catches missing fields at compile time

Potential Issues: ⚠️ One concern

Issue: doc handling for Enum may be inconsistent

In avro/src/schema/parser.rs:616, the get_custom_attributes() function already excludes "doc" from custom attributes globally:

"type" | "name" | "namespace" | "doc" | "aliases" | "logicalType" => continue,

This means doc is already excluded from custom attributes for all schema types. However, the PR adds serialization of doc for Enum schemas in the Serialize implementation. This is correct behavior, but raises a question:

Question: Is there a specific reason why doc serialization was added for Enum but not for Fixed? Looking at the code:

FixedSchema serializes doc at line 301-303 (already present before this PR)
EnumSchema now serializes doc at lines 832-834 (added by this PR)

This appears consistent, but I wanted to verify this was intentional. The test avro_rs_460_enum_default_in_custom_attributes checks that doc is preserved (line 5211), which suggests this is correct.

Performance: ✅ No concerns

No performance impact; changes are in parsing and serialization logic
No additional allocations or computational complexity introduced

Security: ✅ No concerns

No security implications
Changes are purely structural and don't affect validation or data handling

Test Coverage: ✅ Excellent

The PR includes comprehensive tests:

avro_rs_460_fixed_default_in_custom_attributes: Tests that Fixed schemas with default don't include it in custom_attributes and properly round-trip through serialization
avro_rs_460_enum_default_in_custom_attributes: Tests that Enum schemas with default and doc don't include them in custom_attributes and properly round-trip through serialization

Both tests verify:

Custom attributes are empty (no pollution from standard fields)
Round-trip serialization preserves the default value
Proper schema type is maintained

Suggestions for test improvement (optional):

Consider testing with a Fixed/Enum that has BOTH default AND actual custom attributes to ensure they coexist properly
Consider edge cases like empty default values

Best Practices: ✅ Follows conventions

The code follows existing patterns:

Consistent with how other schema types handle standard vs custom attributes
Matches the pattern used for array/map custom attributes (see avro_rs_292 tests)
Uses the same get_custom_attributes() exclusion pattern

Recommendations

Verify doc handling: Confirm that the addition of doc serialization for Enum schemas is intentional and matches the Avro specification
Consider additional test: Add a test case with both default and custom attributes to ensure they work together
Documentation: If this fixes issue fix: Enum and Fixed would put default in custom_attributes apache/avro-rs#460, ensure it's properly referenced in commit messages or PR description

Verdict: ✅ Approve with minor verification

This is a solid fix that addresses a real bug in schema attribute handling. The implementation is clean, well-tested, and follows existing patterns. The only item needing verification is whether the doc field handling for Enum schemas is intentional (I believe it is correct, but worth confirming against the Avro spec).

🤖 Review generated by Claude Code

gemini-code-assist

Code Review

This pull request correctly fixes an issue where default attributes for Enum and Fixed schemas were being incorrectly handled as custom attributes. The changes in parsing and serialization logic are sound, and the addition of new tests ensures the fix is working as expected. I've found one minor issue in a test case that I've commented on.

gemini-code-assist · 2026-02-11T20:34:44Z

avro/src/schema/mod.rs

+        let schema2 = Schema::parse_str(&json)?;
+
+        let Schema::Enum(enum_schema) = schema2 else {
+            panic!("Expected Schema::Fixed, got {schema2:?}");


The panic message here seems to be copy-pasted from the avro_rs_460_fixed_default_in_custom_attributes test. It should refer to Schema::Enum instead of Schema::Fixed to be accurate for this test case.

Suggested change

panic!("Expected Schema::Fixed, got {schema2:?}");

panic!("Expected Schema::Enum, got {schema2:?}");

value:useful; category:bug; feedback: The Gemini AI is correct! The panic message is copied from the unit test for Fixed schema. It should be fixed to say Enum instead!

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@avro/src/schema/mod.rs`:
- Around line 5207-5208: The panic message is a copy-paste error: when matching
let Schema::Enum(enum_schema) = schema2 the panic should state "Expected
Schema::Enum" (and include schema2 debug) instead of "Expected Schema::Fixed";
update the panic in that else branch to panic!("Expected Schema::Enum, got
{schema2:?}") so it accurately reflects the pattern being matched (reference
symbols: Schema::Enum, schema2, enum_schema).

coderabbitai · 2026-02-11T20:35:21Z

avro/src/schema/mod.rs

+        let Schema::Enum(enum_schema) = schema2 else {
+            panic!("Expected Schema::Fixed, got {schema2:?}");


⚠️ Potential issue | 🟡 Minor

Copy-paste error in panic message.

The panic message says "Expected Schema::Fixed" but this test is checking for Schema::Enum.

Proposed fix

- panic!("Expected Schema::Fixed, got {schema2:?}"); + panic!("Expected Schema::Enum, got {schema2:?}");

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

let Schema::Enum(enum_schema) = schema2 else {

panic!("Expected Schema::Fixed, got {schema2:?}");

let Schema::Enum(enum_schema) = schema2 else {

panic!("Expected Schema::Enum, got {schema2:?}");

🤖 Prompt for AI Agents

In `@avro/src/schema/mod.rs` around lines 5207 - 5208, The panic message is a copy-paste error: when matching let Schema::Enum(enum_schema) = schema2 the panic should state "Expected Schema::Enum" (and include schema2 debug) instead of "Expected Schema::Fixed"; update the panic in that else branch to panic!("Expected Schema::Enum, got {schema2:?}") so it accurately reflects the pattern being matched (reference symbols: Schema::Enum, schema2, enum_schema).

value:useful; category:bug; feedback: The CodeRabbit AI is correct! The panic message is copied from the unit test for Fixed schema. It should be fixed to say Enum instead!

augmentcode · 2026-02-11T20:36:16Z

🤖 Augment PR Summary

Summary: Fixes Enum/Fixed schema handling so the Avro-standard default property is not treated as a custom attribute.

Changes:

Serialize default for FixedSchema and serialize both default/doc for EnumSchema.
Update the parser to exclude default from custom_attributes for enum/fixed schemas.
Add regression tests to ensure round-tripping preserves default (and doc for enums) and leaves custom_attributes empty.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 2 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-02-11T20:36:17Z

avro/src/schema/mod.rs

+        let schema2 = Schema::parse_str(&json)?;
+
+        let Schema::Enum(enum_schema) = schema2 else {
+            panic!("Expected Schema::Fixed, got {schema2:?}");


This panic message says Schema::Fixed but the pattern being matched is Schema::Enum, so failures here will be misleading.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

value:useful; category:bug; feedback: The Augment AI is correct! The panic message is copied from the unit test for Fixed schema. It should be fixed to say Enum instead!

augmentcode · 2026-02-11T20:36:17Z

avro/src/schema/parser.rs

            size: size as usize,
            default,
-            attributes: self.get_custom_attributes(complex, vec!["size"]),
+            attributes: self.get_custom_attributes(complex, vec!["size", "default"]),


Since "default" is now excluded from custom_attributes, a non-string fixed "default" currently gets silently dropped (parsed as None) instead of being reported. Consider erroring when "default" exists but isn’t a JSON string so invalid schemas don’t parse successfully.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

value:good-but-wont-fix; category:bug; feedback: The Augment AI says that the value should be a String, but https://avro.apache.org/docs/1.12.0/specification/#fixed does not mention "default" field for Fixed schema at all. For Array schema the default field has JSON array type - https://avro.apache.org/docs/1.12.0/specification/#arrays. The same could be used for Fixed too if Fixed could have a default.

fix: Enum and Fixed would put default in custom_attributes

6b3f851

gemini-code-assist bot reviewed Feb 11, 2026

View reviewed changes

coderabbitai bot reviewed Feb 11, 2026

View reviewed changes

augmentcode bot reviewed Feb 11, 2026

View reviewed changes

	panic!("Expected Schema::Fixed, got {schema2:?}");
	panic!("Expected Schema::Enum, got {schema2:?}");

		let Schema::Enum(enum_schema) = schema2 else {
		panic!("Expected Schema::Fixed, got {schema2:?}");

Conversation

martin-augment commented Feb 11, 2026

Uh oh!

gemini-code-assist bot commented Feb 11, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

coderabbitai bot commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

claude bot commented Feb 11, 2026

PR Review: Fix Enum and Fixed Default in Custom Attributes

Summary

Code Quality: ✅ Good

Potential Issues: ⚠️ One concern

Performance: ✅ No concerns

Security: ✅ No concerns

Test Coverage: ✅ Excellent

Best Practices: ✅ Follows conventions

Recommendations

Verdict: ✅ Approve with minor verification

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

martin-augment Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

martin-augment Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode bot commented Feb 11, 2026

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

martin-augment Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

martin-augment Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Feb 11, 2026 •

edited

Loading